implemented method getSalaryInfo(...) according to requirements #1334

opdhk · 2024-09-23T20:41:11Z

implemented method getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) according to requirements of README.MD

…ADME.MD

mateacademy-ai-mentor

Hey there! 👋 It looks like there's a critical issue with the getSalariesIndexByName method where it might not work correctly if the employeeName appears as a substring of another name. This could lead to incorrect index retrieval, which is pretty important to get right. So, let's fix that to ensure your code is matching names accurately. Keep up the good work, and with this change, your code will be even more robust! 💪

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-09-23T20:41:32Z

src/main/java/core/basesyntax/SalaryInfo.java

+                continue;
+            }
+            LocalDate parsedRecordDate = parseStringToLocalDate(records[DATE_INDEX]);
+            if (parsedRecordDate.isBefore(parseStringToLocalDate(dateFrom))) {


You're parsing 'dateFrom' inside a loop, which is not necessary. You can parse it once before the loop starts, store it in a variable, and then use that variable inside the loop to compare with 'parsedRecordDate'. This will improve performance by avoiding repeated parsing of the same date.

mateacademy-ai-mentor · 2024-09-23T20:41:32Z

src/main/java/core/basesyntax/SalaryInfo.java

+            if (parsedRecordDate.isBefore(parseStringToLocalDate(dateFrom))) {
+                continue;
+            }
+            if (parsedRecordDate.isAfter(parseStringToLocalDate(dateTo))) {


Similar to the previous comment, 'dateTo' is also parsed inside the loop. It's more efficient to parse it once outside the loop and use the parsed value for comparison inside the loop.

mateacademy-ai-mentor · 2024-09-23T20:41:33Z

src/main/java/core/basesyntax/SalaryInfo.java

+        StringBuilder reportBuilder = new StringBuilder(REPORT_HEADER).append(dateFrom)
+                .append(REPORT_DELIMITER)
+                .append(dateTo)


The 'REPORT_DELIMITER' constant is used to separate 'dateFrom' and 'dateTo' in the report. However, it is also hardcoded on line 66. To maintain consistency and avoid magic strings, you should use 'REPORT_DELIMITER' on line 66 as well.

mateacademy-ai-mentor · 2024-09-23T20:41:33Z

src/main/java/core/basesyntax/SalaryInfo.java

+                reportBuilder.append(System.lineSeparator());
+            }
+        }
+        return String.valueOf(reportBuilder);


There is no need to use 'String.valueOf(reportBuilder)' since 'reportBuilder' is already a StringBuilder and calling 'toString()' on it will give you the string representation. You can simply return 'reportBuilder.toString()'.

mateacademy-ai-mentor · 2024-09-23T20:41:33Z

src/main/java/core/basesyntax/SalaryInfo.java

+        return composeReport(names, dateFrom, dateTo, salaries);
+    }
+
+    private static int getSalariesIndexByName(String indexedNameString, String employeeName) {


The method 'getSalariesIndexByName' might not work correctly if the 'employeeName' appears in the indexedNameString as a substring of another name. This could lead to incorrect index retrieval. You should consider adding delimiters to ensure that the exact name is matched.

* parsing dateFrom and dateTo transferred outside the loop; * hardcoded delimiter ' - ' changed to constant; * employee name with delimiters used to prevent situation for incorrect index search if employee name is a substring for another employee name * recommendation to change stringBuilder.toString() instead of String.valueOf(stringBuilder) ignored

mateacademy-ai-mentor

Great job on addressing the core functionality of your code! 🌟 You've handled the main tasks well. For future improvements, consider enhancing your error messages with actual input values to make debugging easier, and use more idiomatic methods like toString() for converting StringBuilder to String. Keep up the good work! 👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-09-24T12:20:02Z

src/main/java/core/basesyntax/SalaryInfo.java

+                reportBuilder.append(System.lineSeparator());
+            }
+        }
+        return String.valueOf(reportBuilder);


Instead of using String.valueOf(reportBuilder), you can simply use reportBuilder.toString() to convert the StringBuilder to a String. This is more idiomatic and direct.

mateacademy-ai-mentor · 2024-09-24T12:20:02Z

src/main/java/core/basesyntax/SalaryInfo.java

+                        Integer.parseInt(records[INCOME_INDEX]) * Integer.parseInt(
+                                records[HOURS_INDEX]);
+            } catch (NumberFormatException e) {
+                throw new RuntimeException(


The error message 'Wrong format of working hours or income per hour in records line.' could be more informative. Include the actual data that caused the error to make debugging easier for users or other developers.

mateacademy-ai-mentor · 2024-09-24T12:20:03Z

src/main/java/core/basesyntax/SalaryInfo.java

+        try {
+            return Integer.parseInt(indexSalaries);
+        } catch (NumberFormatException e) {
+            throw new RuntimeException("Wrong index format in indexed string of employee names.");


Similar to the previous comment, the error message 'Wrong index format in indexed string of employee names.' could include the actual string that failed to parse to aid in debugging.

Elena-Bruyako · 2024-09-24T14:07:50Z

src/main/java/core/basesyntax/SalaryInfo.java

+    private static final String INDEX_NAME_PAIRS_DELIMITER = "|";
+    private static final String REPORT_DELIMITER = " - ";
+    private static final String REPORT_HEADER = "Report for period ";
+
    public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) {


Suggested change

public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) {

public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) {

final LocalDate fromDate = LocalDate.parse(dateFrom, inputFormatter);

final LocalDate toDate = LocalDate.parse(dateTo, inputFormatter);

StringBuilder report = new StringBuilder("Report for period ")

.append(fromDate.format(inputFormatter))

.append(" - ")

.append(toDate.format(inputFormatter));

int[] salaries = new int[names.length];

for (String info : data) {

String[] parts = info.split(" ");

LocalDate recordDay = LocalDate.parse(parts[DATE_INDEX], inputFormatter);

if ((recordDay.isAfter(fromDate) || recordDay.isEqual(fromDate))

&& (recordDay.isBefore(toDate) || recordDay.isEqual(toDate))) {

for (int i = 0; i < names.length; i++) {

if (names[i].equals(parts[NAME_INDEX])) {

salaries[i] += Integer.parseInt(parts[HOURS_INDEX])

* Integer.parseInt(parts[INCOME_INDEX]);

}

}

}

}

for (int i = 0; i < names.length; i++) {

report.append(System.lineSeparator())

.append(names[i])

.append(" - ")

.append(salaries[i]);

}

return report.toString();

}

ahoienko

Nice, left some comments

ahoienko · 2024-09-25T19:11:28Z

src/main/java/core/basesyntax/SalaryInfo.java

+        final LocalDate fromDate = LocalDate.parse(dateFrom, FORMATTER);
+        final LocalDate toDate = LocalDate.parse(dateTo, FORMATTER);
+        StringBuilder report = new StringBuilder(REPORT_HEADER).append(dateFrom)
+                .append(REPORT_DELIMITER)
+                .append(dateTo);
+        int[] salaries = new int[names.length];
+        for (String info : data) {
+            String[] parts = info.split(DATA_DELIMITER);
+            LocalDate recordDay = LocalDate.parse(parts[DATE_INDEX], FORMATTER);
+            if ((recordDay.isAfter(fromDate) || recordDay.isEqual(fromDate)) && (
+                    recordDay.isBefore(toDate) || recordDay.isEqual(toDate))) {
+                for (int i = 0; i < names.length; i++) {
+                    if (names[i].equals(parts[NAME_INDEX])) {
+                        salaries[i] += Integer.parseInt(parts[HOURS_INDEX]) * Integer.parseInt(
+                                parts[INCOME_INDEX]);
+                    }
+                }
+            }
+        }
+        for (int i = 0; i < names.length; i++) {
+            report.append(System.lineSeparator())
+                    .append((names[i]))
+                    .append(REPORT_DELIMITER)
+                    .append(salaries[i]);
+        }
+        return report.toString();


This method is vert complicated, I suggest you to split it into several parts, for example create method that will create report (1), method that will identify is date correct (2) , method that will pars salary (3)

ahoienko · 2024-09-25T19:11:33Z

src/main/java/core/basesyntax/SalaryInfo.java

+        for (int i = 0; i < names.length; i++) {
+            report.append(System.lineSeparator())
+                    .append((names[i]))
+                    .append(REPORT_DELIMITER)
+                    .append(salaries[i]);
+        }


ahoienko · 2024-09-25T19:11:42Z

src/main/java/core/basesyntax/SalaryInfo.java

+            if ((recordDay.isAfter(fromDate) || recordDay.isEqual(fromDate)) && (
+                    recordDay.isBefore(toDate) || recordDay.isEqual(toDate))) {


ahoienko · 2024-09-25T19:11:53Z

src/main/java/core/basesyntax/SalaryInfo.java

+                for (int i = 0; i < names.length; i++) {
+                    if (names[i].equals(parts[NAME_INDEX])) {
+                        salaries[i] += Integer.parseInt(parts[HOURS_INDEX]) * Integer.parseInt(
+                                parts[INCOME_INDEX]);
+                    }
+                }


implemented method getSalaryInfo(...) according to requirements of RE…

8ea1383

…ADME.MD

mateacademy-ai-mentor suggested changes Sep 23, 2024

View reviewed changes

opdhk requested a review from mateacademy-ai-mentor September 24, 2024 12:19

mateacademy-ai-mentor approved these changes Sep 24, 2024

View reviewed changes

Elena-Bruyako suggested changes Sep 24, 2024

View reviewed changes

changed according to the reviewer's fixes

6e8dfe4

opdhk requested a review from Elena-Bruyako September 25, 2024 18:59

ahoienko suggested changes Sep 25, 2024

View reviewed changes

2nd iteration of changes according to the reviewer's fixes

9e53943

opdhk requested a review from ahoienko September 26, 2024 19:19

Elena-Bruyako approved these changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented method getSalaryInfo(...) according to requirements #1334

implemented method getSalaryInfo(...) according to requirements #1334

opdhk commented Sep 23, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Sep 23, 2024

mateacademy-ai-mentor Sep 23, 2024

mateacademy-ai-mentor Sep 23, 2024

mateacademy-ai-mentor Sep 23, 2024

mateacademy-ai-mentor Sep 23, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Sep 24, 2024

mateacademy-ai-mentor Sep 24, 2024

mateacademy-ai-mentor Sep 24, 2024

Elena-Bruyako Sep 24, 2024

ahoienko left a comment

ahoienko Sep 25, 2024

ahoienko Sep 25, 2024

ahoienko Sep 25, 2024

ahoienko Sep 25, 2024

		if ((recordDay.isAfter(fromDate) \|\| recordDay.isEqual(fromDate)) && (
		recordDay.isBefore(toDate) \|\| recordDay.isEqual(toDate))) {

implemented method getSalaryInfo(...) according to requirements #1334

Are you sure you want to change the base?

implemented method getSalaryInfo(...) according to requirements #1334

Conversation

opdhk commented Sep 23, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoienko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment